feat: add MC AKS node health metrics to gather-observability - #6676
feat: add MC AKS node health metrics to gather-observability#6676Gerd Oberlechner (geoberle) wants to merge 2 commits into
Conversation
1460cb0 to
0b3e24f
Compare
There was a problem hiding this comment.
Pull request overview
Adds an “MC AKS Metrics” observability panel for management-cluster node health and resource usage.
Changes:
- Adds node phase and unhealthy-condition charts.
- Adds CPU, memory, disk, and pod-capacity charts.
- Applies management-cluster filtering and replica deduplication.
Suppressed comments (5)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:344
- As with the CPU query,
instanceis a node-exporter scrape address rather than the Kubernetes node name, so this selector does not remove system/infra nodes. The resulting maximum memory value can therefore be driven by an excluded pool and does not satisfy the chart's user-node scope. Map the series throughkubelet_node_nameor otherwise apply the pool filter using the actual node label before taking the max.
and on (node, cluster)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:357
- This
instancefilter has the same identity mismatch: node-exporter exposes the scrape endpoint ininstance, not the AKS node name. System/infra root filesystems therefore remain in the max, so a full system disk can falsely report user-node disk pressure. Apply the pool exclusion after mapping the node-exporter series to the Kubernetes node (for example viakubelet_node_name).
),
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:254
- Because this PR adds a metrics visualization, the PR description must include the required before/after screenshots (with annotations if needed) and a linked Jira/GitHub tracking issue or an explanation for why no issue exists, per the Pull Request Standards. Please update the description before approval.
)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:254
- This PR changes metrics visualizations by adding six charts. CONTRIBUTING.md requires before/after screenshots for graph, dashboard, and metrics-visualization changes; the current PR description does not include them. Please attach the required screenshots (and annotate any subtle changes) before approval.
)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:254
- The PR description does not reference a Jira/GitHub tracking issue. CONTRIBUTING.md requires every PR to include a ticket link or explain why no ticket exists; please add that reference or explanation before approval.
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:437
kubelet_running_podsis keyed by the kubelet scrape targetinstance, whilekube_node_status_capacityis keyed by Kubernetesnode. Relabeling the capacity node intoinstancedoes not join these series (the repository's existing KubeletTooManyPods rule useskubelet_node_namefor this mapping), so this query will normally return no pod-capacity data. Join running pods tokubelet_node_nameon(cluster, instance), then divide by capacity on(cluster, node)and apply the user-node filter there.
label_replace(
kube_node_status_capacity{cluster=~".*-mgmt-.*", node!~"aks-system-.*|aks-infra.*-.*", resource="pods"},
"instance", "$1", "node", "(.*)"
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:323
- The test plan is entirely unchecked, so the PR provides no evidence that the required parser test or the chart/data smoke checks passed. Please run the listed validation and update the checklist, or document any unrelated/flaky failure, before requesting approval.
- title: "MC AKS Metrics"
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:395
- Filtering by
instancedoes not select Kubernetes node names in this Prometheus setup. The existing node-exporter rules map each target tonodevia(cluster, namespace, pod), and repository fixtures show endpoint-style instances such as10.0.0.4:9100; consequently this matcher does not exclude system/infra nodes and the cluster maximum can report a non-user node. Join to the node label before filtering and aggregating (or use an equivalent recording rule).
rate(node_cpu_seconds_total{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*", mode="idle"}[5m])
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:409
- These memory operands apply the pool exclusion to
instance, but node-exporterinstanceis the scrape target identity rather than the Kubernetes node label in this deployment. System/infra nodes can therefore contribute to the maximum despite the chart claiming to show user nodes. Map the node-exporter target tonodefirst, then filter/aggregate by that label.
max by (instance, cluster) (node_memory_MemAvailable_bytes{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*"})
/
max by (instance, cluster) (node_memory_MemTotal_bytes{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*"})
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:422
- The root-filesystem operands have the same incorrect pool filter on scrape-target
instanceinstead of Kubernetesnode. This can include system/infra filesystems in the cluster maximum and make the user-node disk chart inaccurate. Attach the node label before filtering and aggregate by node.
max by (instance, cluster) (node_filesystem_avail_bytes{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*", mountpoint="/", fstype!="tmpfs"})
/
max by (instance, cluster) (node_filesystem_size_bytes{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*", mountpoint="/", fstype!="tmpfs"})
|
/lgtm |
0b3e24f to
5dfc8bf
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bennerv, geoberle, raelga The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold Revision 5dfc8bf was retested 3 times: holding |
5dfc8bf to
4940e38
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:347
- This
label_replacecall uses an empty source label name and regex ("", ""), which is likely invalid PromQL and can cause the chart query to fail at runtime. Use an existing label (e.g.cluster) as the source with.*so the constantphaselabel is added safely.
"phase", "NotReady", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:358
- This
label_replacecall uses an empty source label name and regex ("", ""), which is likely invalid PromQL and can cause the chart query to fail at runtime. Use an existing label (e.g.cluster) as the source with.*so the constantphaselabel is added safely.
"phase", "Joining", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:323
- This PR adds new metrics visualizations (a new Spyglass panel with multiple charts). CONTRIBUTING.md Pull Request Standards require before/after screenshots for graph/UI/metrics changes; the PR description should include those screenshots before merge.
- title: "MC AKS Metrics"
4940e38 to
a322a7c
Compare
a322a7c to
e56a9fe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:336
- In PromQL,
label_replace()requires a non-empty source label and regex. Using empty strings here will make the query fail to parse/execute, so the “Ready” phase series won’t render.
"phase", "Ready", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:347
- In PromQL,
label_replace()requires a non-empty source label and regex. Using empty strings here will make the query fail to parse/execute, so the “NotReady” phase series won’t render.
"phase", "NotReady", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:358
- In PromQL,
label_replace()requires a non-empty source label and regex. Using empty strings here will make the query fail to parse/execute, so the “Joining” phase series won’t render.
"phase", "Joining", "", ""
test/cmd/aro-hcp-tests/gather-observability/chart.go:210
series[i].labelisn’t populated until afterfindCommonLabels()/compactMetricLabel(). Sorting before labels are assigned means all comparisons are""vs"", so the sort can be non-deterministic and won’t achieve consistent color assignment.
switch q.ChartType {
case chartTypeFacetedStackedArea:
cd := buildFacetedStackedAreaChartData(q, resourceID, series, tw)
cd.Warning = warning
return cd
case chartTypeLine:
cd := buildLineChartData(q, resourceID, series, tw)
cd.Warning = warning
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:325
- This PR changes metrics visualizations (adds a new metrics panel). Per CONTRIBUTING.md Pull Request Standards, please add before/after screenshots of the rendered Spyglass charts to the PR description.
- title: "MC AKS Metrics"
queries:
- title: "MC User Node Count by Phase"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:327
- This PR adds/changes metrics visualizations (new gather-observability panel). CONTRIBUTING.md Pull Request Standards require before/after screenshots for graph/metrics changes, but the PR description currently only includes a test plan. Please add before/after screenshots (or explain why not applicable) to meet the repo PR standards.
- title: "MC AKS Metrics"
queries:
- title: "MC User Node Count by Phase"
description: "User node count per management cluster, stacked by phase: Ready (healthy, age >10m), NotReady (unhealthy, age >10m), Joining (any status, age ≤10m). Excludes system and infra pool nodes. Each sub-chart shows one management cluster."
query: |
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:432
- The PR description says the Pod Capacity chart should be the maximum running/capacity ratio across user nodes per management cluster, but this query computes an overall cluster ratio (sum(running)/sum(capacity)). Either update the PR description to match, or (recommended) change this query/description to plot the per-node ratio and take the max per cluster.
- title: "MC User Node Pod Capacity"
description: "Overall pod utilization across user nodes per management cluster. Computed as total running pods / total pod capacity. Values approaching 100% mean the cluster is running out of pod capacity."
query: |
sum by (cluster) (
max by (instance, cluster) (kubelet_running_pods{cluster=~".*-mgmt-.*", instance!~"aks-system-.*|aks-infra.*-.*"})
e56a9fe to
baf730b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/cmd/aro-hcp-tests/gather-observability/chart.go:224
- The new series sort runs before
series[i].labelis populated (labels are set later viacompactMetricLabel), so this sort is effectively a no-op and won't stabilize legend/color ordering. Move the sort to after the label-compaction loop and sort by the final display label.
// Sort by label for consistent color assignment across charts
slices.SortFunc(series, func(a, b parsedSeries) int {
return cmp.Compare(a.label, b.label)
})
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:336
label_replacevalidates thatsrc_labelis a valid label name; passing an empty string here will cause a PromQL evaluation error, so the whole panel query will fail. Use an always-present label (e.g.cluster) as the source to set a constantphaselabel.
"phase", "Ready", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:347
- Same issue as the earlier
label_replace: the emptysrc_labelis not a valid PromQL label name and will make the query fail. Use an existing label likeclusterto set a constantphase.
"phase", "NotReady", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:358
- Same issue as the earlier
label_replace: using an emptysrc_labelis invalid PromQL and will break the query. Use an always-present label (e.g.cluster) as the source when setting a constantphase.
"phase", "Joining", "", ""
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:323
- This PR adds new metrics visualizations; per CONTRIBUTING.md Pull Request Standards §5 (screenshots for graph/UI/metrics changes), the PR description should include before/after screenshots of the Spyglass output for the new panel (or explain why before/after isn't meaningful).
- title: "MC AKS Metrics"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:327
- CONTRIBUTING.md requires before/after screenshots for any graph/UI/metrics/visualization change. This PR adds a new Spyglass metrics panel (and adjusts percent-axis rendering), but the PR description currently doesn’t include screenshots. Please attach screenshots showing the new “MC AKS Metrics” panel rendering with data (and any relevant before/after comparison if applicable).
- title: "MC AKS Metrics"
queries:
- title: "MC User Node Count by Phase"
description: "User node count per management cluster, stacked by phase: Ready (healthy, age >10m), NotReady (unhealthy, age >10m), Joining (any status, age ≤10m). Excludes system and infra pool nodes. Each sub-chart shows one management cluster."
query: |
Add a new "MC AKS Metrics" panel to the gather-observability Spyglass artifacts with six charts for management cluster node health visibility: - Node count by phase (Ready/NotReady/Joining) — faceted per MC - Unhealthy node conditions — faceted per MC, only non-zero conditions - CPU, memory, disk usage — max across user nodes per MC - Pod capacity utilization — max across user nodes per MC All queries filter to management clusters (cluster=~".*-mgmt-.*") and exclude system/infra pool nodes. Queries handle HA replica dedup and dual kube-state-metrics instance dedup to avoid double-counting.
The sort for consistent color assignment ran before compactMetricLabel populated the label field, making it a no-op on empty strings.
61a69e0 to
11bac85
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/cmd/aro-hcp-tests/gather-observability/queries.yaml:323
- CONTRIBUTING.md Pull Request Standards require before/after screenshots for graph/UI/metrics/performance changes. This PR adds a new gather-observability metrics panel, but the PR description currently doesn’t include screenshots; please add before/after screenshots (or clearly note there is no meaningful “before” and include an “after” screenshot) so reviewers can validate the visualization change.
- title: "MC AKS Metrics"
Summary
Jira: https://issues.redhat.com/browse/ARO-29285
cluster=~".*-mgmt-.*") and exclude system/infra pool nodesTest plan
go test ./test/cmd/aro-hcp-tests/gather-observability/...passes (validates queries.yaml parsing)